-
-
Notifications
You must be signed in to change notification settings - Fork 360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix missing command temp-directory #928
Conversation
Sync repo
After changing tempDirectory to tempDir, with a fallback for tempDirectory, the default values set in the 'temp-dir' yargs option were being used rather than falling back to tempDirectory and then the default temp dir. This change removes the yargs default value for the option 'temp-dir'. This means the statement that assigns '_tempDirectory' in 'index.js' attempts to use temp-dir, then temp-directory, then the default value. This is now under test with some new test cases that show: - tempDir is preferred to tempDirectory - tempDirectory is used if tempDir isn't set - that we fall back to the '.nyc_output' dir if neither tempDir or tempDirectory are set.
One concern I hadn't previously thought of, this will stop yargs from displaying the default value in this._tempDirectory = config.tempDirectory || config.tempDir This would use Sorry I didn't think of this at first. @AndrewFinlay @bcoe @JaKXz do you have any opinion on this? |
It did cross my mind that you'd lose the default setting in the help, but I also thought it to be important to prefer Looking at the yargs docs there is the opt key So I suppose if preferring With a best case solution in mind, and I think we may have discussed it before, adding something like a |
I was not aware of the As for preferring temporaryDir over tempDir this might actually be better. Old versions of nyc |
Sounds good, will move it over to that. |
Maintains notice of default temp-dir in help output. Removed tests as I'm not confident with these right now, will take another look when I get some time.
@simlu Could you test this updated patch, confirm that it resolves your issue? Thanks! @AndrewFinlay thanks for your quick responses to this issue! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending feedback from issue reporter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense! too bad we didn't catch it the first time. Would it be possible to write a test case? 🤷♂️
This has been on my list for a while now, it hasn't been my highest day job priority but since it's the quiet season I had planned to get onto this next week. Anyway I had a few troubles getting the tests up and running on my first attempt at this and dumped them as they weren't right. So I'll try and get them working properly and submit them with this fix. It'll need a refactor too as it's a pretty messy commit history so far. |
@coreyfarrell This was so breaking for me that I had to fix it immediately - this fix came to late unfortunately. So I'm not really able to test at this point if the old syntax still works as expected - the new one however does the job. |
Don't worry about the commit history, we'll do a squash when we merge. @bcoe @coreyfarrell can we just merge this as is and let @AndrewFinlay write a test in a later PR? |
Hi all,
So I've come up with this and added a few tests to confirm behaviour.
Let me know if anything more needs to be done to get this over the line.
Thanks,
Andrew